VIF #26
VIF #26
Conversation
Codecov ReportPatch has no changes to coverable lines.
📢 Thoughts on this report? Let us know!. |
src/regressionmodel.jl
Outdated
| is not particularly informative anyway. | ||
| """ | ||
| function vif(model::RegressionModel) | ||
| mm = Statistics.cov2cor!(vcov(model), stderror(model)) |
There was a problem hiding this comment.
If the model stores its covariance matrix directly such that vcov(model) is non-copying, this call could effectively invalidate the model object.
Co-authored-by: Alex Arslan <[email protected]>
src/regressionmodel.jl
Outdated
| # This generic function is owned by StatsModels.jl, which is the sole provider | ||
| # of the default definition. StatsModels needs to own the default definition | ||
| # because it depends on the @formula interface. |
There was a problem hiding this comment.
Maybe we should only define gvif in StatsModels? It seems unlikely that modeling packages will define custom gvif functions without depending on StatsModels given that it depends on formulas.
There was a problem hiding this comment.
That's fine with me -- and we can always move it here if we decide that's appropriate in the future.
There was a problem hiding this comment.
I'm almost tempted to move vif to StatsModels too. We have a tradition of defining simple generic fallbacks in StatsAPI which is somewhat unfortunate as StatsAPI is supposed to be an interface package. Currently the two largest functions are r2 and adjr2, which are still relatively short. vif would be about as large. Opinions?
There was a problem hiding this comment.
fine by me, that seems like a reasonable home for it given that StatsModels is not trying to wrap things without being a direct dependency.
There was a problem hiding this comment.
I do think that having teh name owned by StatsAPI is good though, just some basic implementation in StatsModels
src/regressionmodel.jl
Outdated
| # This generic function is owned by StatsModels.jl, which is the sole provider | ||
| # of the default definition. StatsModels needs to own the default definition | ||
| # because it depends on the @formula interface. |
There was a problem hiding this comment.
I'm almost tempted to move vif to StatsModels too. We have a tradition of defining simple generic fallbacks in StatsAPI which is somewhat unfortunate as StatsAPI is supposed to be an interface package. Currently the two largest functions are r2 and adjr2, which are still relatively short. vif would be about as large. Opinions?
|
@nalimilan The name vif should still be owned by StatsAPI, but I could see moving the implementation to StatsBase. We don't actually need any of the StatsModels machinery for normal VIF and StatsModels is a fairly big package. But also the implementation here is not much code at all and only depends on Statistics, which I'm guessing most users have loaded if they're using this package. |
|
StatsBase doesn't contain code related to models anymore (it just reexports definitions from StatsAPI for historical reasons), and anyway most of it will be merged into Statistics as soon as we make it an upgradable stdlib. So I'd either put this in StatsAPI or in StatsModels. We were precisely discussing the acceptability of having StatsAPI depend on Statistics with @bkamins at JuliaStats/Statistics.jl#87 (comment). |
|
@nalimilan if the dependency on Statistics is part of the big question here, then it's easy enough to the row normalization directly and not use |
|
After the discussion here and further discussion with @kleinschmidt, I think StatsAPI should have both the |
test/regressionmodel.jl
Outdated
| end # module TestRegressionModel |
There was a problem hiding this comment.
You should also be able to revert changes to this file and the next one.
There was a problem hiding this comment.
the missing terminal newlines? I think those should be required FWIW
Co-authored-by: Milan Bouchet-Valat <[email protected]>
as discussed here: JuliaStats/GLM.jl#428 (comment)